-
Couldn't load subscription status.
- Fork 36
InitContext, part 5 - Remove SamplingContext, SampleFrom{Prior,Uniform}, {tilde_,}assume
#985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
c32d112 to
b7221cc
Compare
InitContext, part 5 - Remove SamplingContext, SampleFromPrior, SampleFromUniform, and associated codeInitContext, part 5 - Remove SamplingContext, SampleFrom{Prior,Uniform}, {tilde_,}assume
b7221cc to
3835d01
Compare
d55d378 to
a392451
Compare
3835d01 to
713034f
Compare
a392451 to
12d93e5
Compare
713034f to
45a97ee
Compare
12d93e5 to
7a8e7e3
Compare
45a97ee to
e817d9c
Compare
7e38bbe to
1d8bceb
Compare
92772b5 to
1ffc409
Compare
1d8bceb to
2edcd10
Compare
1957b06 to
4fc60dc
Compare
Benchmark Report for Commit f4e5f4bComputer InformationBenchmark Results |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #985 +/- ##
============================================
+ Coverage 81.20% 82.13% +0.92%
============================================
Files 39 39
Lines 3910 3811 -99
============================================
- Hits 3175 3130 -45
+ Misses 735 681 -54 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
DynamicPPL.jl documentation for PR #985 is available at: |
3a16f9c to
4c96020
Compare
3951d1d to
79a5f3c
Compare
| end | ||
| end | ||
|
|
||
| @testset "Turing#2151: ReverseDiff compilation & eltype(vi, spl)" begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find a way to reproduce this test without SamplingContext and the whole tilde-pipeline machinery that this PR removes, so I think that this is no longer relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually just for posterity I did find a way to reproduce it when I tried to remove eltype etc. in #1015. But without the changes from that PR I couldn't repro it
79a5f3c to
7aae613
Compare
4c96020 to
7b4c5fa
Compare
0bd4e02 to
8e3dcac
Compare
f6dd1d5 to
d9292ad
Compare
726d486 to
bc04355
Compare
8e3dcac to
0b87d0d
Compare
0b87d0d to
992569f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry this file's diff is a bit of a mess, there are no real code changes, it's just:
- merged
tilde_assume,tilde_assume!!, andassumeinto a single function (they all just called each other) - added types to the arguments
- added proper docstrings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enjoyed this one a lot.
| ## Model | ||
|
|
||
| ### Macros | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it time to start a HISTORY.md entry? Might be easier to do it here were you can cross-check against what's being removed, rather than once everything is in breaking in a huge diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good idea to do it in this PR. I'll write one up later and ping you again
| end | ||
| function tilde_assume(rng::Random.AbstractRNG, ::DefaultContext, sampler, right, vn, vi) | ||
| return assume(rng, sampler, right, vn, vi) | ||
| function tilde_assume!!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm that I understand how this will play out in Turing.jl: The idea is that samplers don't need to modify the behaviour of the tilde pipeline any more, and thus SamplingContext can go in its entirety, and we don't need things like tilde_assume without !! or assume. And the few that do still need to do that (Gibbs, maybe particles, hopefully nothing else) need to define their own context. Is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, although I'm not 100% sure. We'll probably have to do a similar process to what we did last time for accumulators: make a Turing PR that builds against the breaking branch of DPPL. Things might break. But I'm hoping it won't be too bad haha.
|
Mildly tangential: I kind of find it weird that we do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I, too, have disliked the bolding taking precedence over headings. Happy for that to be changed.
|
|
||
| - `InitContext(rng, InitFromPrior())`: New values are sampled from the prior distribution (on the right-hand side of the tilde). | ||
| - `InitContext(rng, InitFromUniform(a, b))`: New values are sampled uniformly from the interval `[a, b]`, and then invlinked to the support of the distribution on the right-hand side of the tilde. | ||
| - `InitContext(rng, InitFromParams(p, fallback))`: New values are obtained by indexing into the `p` object, which can be a `NamedTuple` or `Dict{<:VarName}`. If a variable is not found in `p`, then the `fallback` strategy is used, which is simply another of these strategies. In particular, `InitFromParams` enables the case where different variables are to be initialised from different sources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enables the case where different variables are to be initialised from different sources.
Does this allow different sources in some broader sense than some having fixed values and others using fallback? Like could I do InitFromPrior for some and InitFromUniform for others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That thought did pop into my mind when I was writing, which led me to mention the custom initialisation strategy just below this. You'd have to write your custom InitFromFoo, but it would be possible in that function to check e.g. the varname and dispatch to prior / uniform accordingly.
Co-authored-by: Markus Hauru <[email protected]>
|
Day N of coveralls failing randomly... |
* Bump minor version * bump benchmarks compat * add a skeletal changelog * `InitContext`, part 3 - Introduce `InitContext` (#981) * Implement InitContext * Fix loading order of modules; move `prefix(::Model)` to model.jl * Add tests for InitContext behaviour * inline `rand(::Distributions.Uniform)` Note that, apart from being simpler code, Distributions.Uniform also doesn't allow the lower and upper bounds to be exactly equal (but we might like to keep that option open in DynamicPPL, e.g. if the user wants to initialise all values to the same value in linked space). * Document * Add a test to check that `init!!` doesn't change linking * Fix `push!` for VarNamedVector This should have been changed in #940, but slipped through as the file wasn't listed as one of the changed files. * Add some line breaks Co-authored-by: Markus Hauru <[email protected]> * Add the option of no fallback for ParamsInit * Improve docstrings * typo * `p.default` -> `p.fallback` * Rename `{Prior,Uniform,Params}Init` -> `InitFrom{Prior,Uniform,Params}` --------- Co-authored-by: Markus Hauru <[email protected]> * use `varname_leaves` from AbstractPPL instead (#1030) * use `varname_leaves` from AbstractPPL instead * add changelog entry * fix import * tidy occurrences of varname_leaves as well (#1031) * `InitContext`, part 4 - Use `init!!` to replace `evaluate_and_sample!!`, `predict`, `returned`, and `initialize_values` (#984) * Replace `evaluate_and_sample!!` -> `init!!` * Use `ParamsInit` for `predict`; remove `setval_and_resample!` and friends * Use `init!!` for initialisation * Paper over the `Sampling->Init` context stack (pending removal of SamplingContext) * Remove SamplingContext from JETExt to avoid triggering `Sampling->Init` pathway * Remove `predict` on vector of VarInfo * Fix some tests * Remove duplicated test * Simplify context testing * Rename FooInit -> InitFromFoo * Fix JETExt * Fix JETExt properly * Fix tests * Improve comments * Remove duplicated tests * Docstring improvements Co-authored-by: Markus Hauru <[email protected]> * Concretise `chain_sample_to_varname_dict` using chain value type * Clarify testset name * Re-add comment that shouldn't have vanished * Fix stale Requires dep * Fix default_varinfo/initialisation for odd models * Add comment to src/sampler.jl Co-authored-by: Markus Hauru <[email protected]> --------- Co-authored-by: Markus Hauru <[email protected]> * `InitContext`, part 5 - Remove `SamplingContext`, `SampleFrom{Prior,Uniform}`, `{tilde_,}assume` (#985) * Remove `SamplingContext` for good * Remove `tilde_assume` as well * Split up tilde_observe!! for Distribution / Submodel * Tidy up tilde-pipeline methods and docstrings * Fix tests * fix ambiguity * Add changelog * Update HISTORY.md Co-authored-by: Markus Hauru <[email protected]> --------- Co-authored-by: Markus Hauru <[email protected]> * fix missing import * Shuffle context code around and remove dead code (#1050) * Delete the `"del"` flag (#1058) * Delete del * Fix a typo * Add HISTORY entry about del * Fixes for Turing 0.41 (#1057) * setleafcontext(model, ctx) and various other fixes * fix a bug * Add warning for `initial_parameters=...` * Remove `resume_from` and `default_chain_type` (#1061) * Remove resume_from * Format * Fix test * remove initial_params warning * Allow more flexible `initial_params` (#1064) * Enable NamedTuple/Dict initialisation * Add more tests * fix include_all kwarg for predict, improve perf (#1068) * Fix `include_all` for predict * Fix include_all for predict, some perf improvements * Replace `Metadata.flags` with `Metadata.trans` (#1060) * Replace Medata.flags with Metadata.trans * Fix a bug * Fix a typo * Fix two bugs * Rename trans to is_transformed * Rename islinked to is_transformed, remove duplication * Change pointwise_logdensities default key type to VarName (#1071) * Change pointwise_logdensities default key type to VarName * Fix a doctest * Fix DynamicPPL / MCMCChains methods (#1076) * Reimplement pointwise_logdensities (almost completely) * Move logjoint, logprior, ... as well * Fix imports, etc * Remove tests that are failing (yes I learnt this from Claude) * Changelog * logpdf * fix docstrings * allow dict output * changelog * fix some comments * fix tests * Fix more imports * Remove stray n Co-authored-by: Markus Hauru <[email protected]> * Expand `logprior`, `loglikelihood`, and `logjoint` docstrings --------- Co-authored-by: Markus Hauru <[email protected]> * Remove `Sampler` and its interface (#1037) * Remove `Sampler` and `initialstep` * Actually just remove the entire file * forgot one function * Move sampling test utils to Turing as well * Update changelog to correctly reflect changes * [skip ci] Make changelog headings more consistent --------- Co-authored-by: Markus Hauru <[email protected]>
Part 1: Adding
hasvalueandgetvalueto AbstractPPLPart 2: Removing
hasvalueandgetvaluefrom DynamicPPLPart 3: Introducing
InitContextandinit!!Part 4: Using
InitFromParamsto implementpredict,returned, andinitialize_valuesThis is part 5/N of #967.
This PR removes everything that is no longer needed.
SamplingContext,SampleFromPrior,SampleFromUniform, now have direct one-to-one replacements (albeit with slightly different behaviour since they now always overwrite variables in the varinfo).It also removes
assumeandtilde_assume.Prior to this PR we had two different kinds of
assume, one with a sampler and one without. Now we only have the one without, so we can just move that definition intotilde_assume!!(::DefaultContext, ...).Finally,
tilde_assumehas been subsumed intotilde_assume!!as we can just dispatch on the type ofright. (Previously this wasn't possible because there was a lot of stuff aboutis_rhs_model, etc. etc. which was removed in #960.)Closes #859
Closes #955